Automated Test: large-image-processing #322
Conversation
📝 WalkthroughWalkthroughThe changes implement a fixed 10 MB download size limit for URL uploads, add iterative image downsizing logic in the uploads controller, and refactor OptimizedImage to accept dimensions as a single string parameter instead of separate width/height arguments. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Controller as UploadsController
participant OptimizedImage
participant Storage as File Storage
User->>Controller: Upload via URL
Controller->>Controller: Download file
Controller->>Controller: Check if image & size > limit
alt Image exceeds max_image_size_kb
loop Up to 5 downsize attempts
Controller->>OptimizedImage: downsize(from, to, dimensions)
OptimizedImage->>OptimizedImage: optimize with reduced dimensions
OptimizedImage->>Storage: Write resized image
OptimizedImage-->>Controller: Success or need further reduction
end
end
Controller->>Storage: Create Upload record
Controller-->>User: Upload complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/assets/javascripts/discourse/lib/utilities.js (1)
180-186:⚠️ Potential issue | 🟡 MinorUse configurable backend upload limits instead of hardcoding 10 MB.
The client-side validation hardcodes 10240 KB (10 MB) for all uploads at lines 180–186 and 246, but the backend exposes configurable
Discourse.SiteSettings.max_image_size_kb(3 MB in tests) andDiscourse.SiteSettings.max_attachment_size_kb(1 MB in tests). This creates a mismatch where the client allows significantly larger files than the server, leading to rejected uploads with misleading error messages. Derive the limit from the appropriate backend setting based on upload type to keep them in sync.
🤖 Fix all issues with AI agents
In `@app/controllers/uploads_controller.rb`:
- Line 55: Replace the hardcoded 10.megabytes used when downloading into
tempfile with the site-setting-based limit to match the rest of the code: use
[SiteSetting.max_image_size_kb,
SiteSetting.max_attachment_size_kb].max.kilobytes for the size argument in the
FileHelper.download call in uploads_controller.rb (the line that assigns
tempfile) while preserving the existing rescue nil behavior.
In `@app/models/optimized_image.rb`:
- Around line 141-156: The duplicate downsize definitions must be consolidated
so callers like resize_emoji (which passes three positional args for
width/height) and uploads_controller (which passes a dimensions string) both
work: remove the duplicated method and implement a single
OptimizedImage.downsize that accepts both signatures (e.g., parameters from, to,
then either (max_width, max_height, opts={}) or (dimensions_string, opts={}),
detect whether the third arg is a String or a Numeric, normalize to a dimensions
string like "WxH", merge opts, and then call optimize(operation, from, to,
dimensions, opts); update references to method names used in optimize (e.g.,
optimize and any *_instructions methods remain unchanged).
🧹 Nitpick comments (2)
app/assets/javascripts/discourse/lib/utilities.js (1)
246-248: Avoid switch-scope leakage in the 413 branch.
var maxSizeKBis function-scoped inside aswitch, which can leak across cases; the linter is right to warn. Wrap the case in a block and useconst(or move the constant outside the switch).Proposed fix
- case 413: - var maxSizeKB = 10 * 1024; // 10 MB - bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB })); - return; + case 413: { + const maxSizeKB = 10 * 1024; // 10 MB + bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB })); + return; + }app/controllers/uploads_controller.rb (1)
63-70: Handle downsize failures to avoid wasted retries and ambiguous errors.
OptimizedImage.downsizereturns a boolean. If it fails, the loop still retries and then proceeds with a too-large file. Consider breaking on failure and returning a clear error when the size still exceeds the limit after the attempts.
| @@ -52,14 +52,23 @@ def create_upload(type, file, url) | |||
| begin | |||
| # API can provide a URL | |||
| if file.nil? && url.present? && is_api? | |||
| tempfile = FileHelper.download(url, SiteSetting.max_image_size_kb.kilobytes, "discourse-upload-#{type}") rescue nil | |||
| tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") rescue nil | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check other download size limits and site setting usage
rg -n "FileHelper\.download\(" -C2 app lib
echo "---"
rg -n "max_image_size_kb|max_attachment_size_kb" -C2 app libRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 8172
Use site settings for the initial download limit instead of hardcoded 10 MB.
Line 55 uses a hardcoded 10.megabytes limit, but the rest of the codebase consistently uses SiteSetting.max_image_size_kb.kilobytes for download size constraints (and the resize loop on lines 66-68 also respects the site setting). This creates a mismatch: if max_image_size_kb is lower, the initial download overshoots the limit; if higher, the download is unnecessarily capped. Use [SiteSetting.max_image_size_kb, SiteSetting.max_attachment_size_kb].max.kilobytes to align with the pattern elsewhere.
🧰 Tools
🪛 RuboCop (1.84.0)
[convention] 55-55: Avoid using rescue in its modifier form.
(Style/RescueModifier)
🤖 Prompt for AI Agents
In `@app/controllers/uploads_controller.rb` at line 55, Replace the hardcoded
10.megabytes used when downloading into tempfile with the site-setting-based
limit to match the rest of the code: use [SiteSetting.max_image_size_kb,
SiteSetting.max_attachment_size_kb].max.kilobytes for the size argument in the
FileHelper.download call in uploads_controller.rb (the line that assigns
tempfile) while preserving the existing rescue nil behavior.
| def self.resize(from, to, width, height, opts={}) | ||
| optimize("resize", from, to, width, height, opts) | ||
| optimize("resize", from, to, "#{width}x#{height}", opts) | ||
| end | ||
|
|
||
| def self.downsize(from, to, max_width, max_height, opts={}) | ||
| optimize("downsize", from, to, max_width, max_height, opts) | ||
| optimize("downsize", from, to, "#{max_width}x#{max_height}", opts) | ||
| end | ||
|
|
||
| def self.optimize(operation, from, to, width, height, opts={}) | ||
| dim = dimensions(width, height) | ||
| def self.downsize(from, to, dimensions, opts={}) | ||
| optimize("downsize", from, to, dimensions, opts) | ||
| end | ||
|
|
||
| def self.optimize(operation, from, to, dimensions, opts={}) | ||
| method_name = "#{operation}_instructions" | ||
| method_name += "_animated" if !!opts[:allow_animation] && from =~ /\.GIF$/i | ||
| instructions = self.send(method_name.to_sym, from, to, dim, opts) | ||
| instructions = self.send(method_name.to_sym, from, to, dimensions, opts) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file and the code at the specified lines
cat -n app/models/optimized_image.rb | sed -n '141,156p'Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 835
🏁 Script executed:
# Search for all downsize call sites in the codebase
rg -n "downsize\(" -C2 app libRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1427
🏁 Script executed:
# Also check for OptimizedImage.downsize specifically
rg -n "OptimizedImage\.downsize\(" -C2Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 861
🏁 Script executed:
# Let me verify the resize_emoji.rb context and opts usage
cat -n app/jobs/regular/resize_emoji.rb | head -20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 578
🏁 Script executed:
# Also check if there are any other downsize calls I might have missed
rg -n "\.downsize\(" --type rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 114
🏁 Script executed:
# Search for all downsize calls without type filter
rg "downsize\(" --color=neverRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 516
Duplicate downsize definitions break existing callers (Ruby has no overloads).
The second downsize definition at line 149 completely overrides the first at line 145. The resize_emoji job calls downsize with the old signature (path, path, 100, 100, opts) (5 positional args), which will fail with the current second definition that expects only 4 parameters: (from, to, dimensions, opts={}). This causes an ArgumentError: wrong number of arguments (given 5, expected 2..4) at runtime.
The uploads_controller call uses the new signature (tempfile.path, tempfile.path, "80%", allow_animation: ...) and will continue to work.
To fix this, either:
- Keep only the second definition and update the
resize_emojijob to pass dimensions as a string:OptimizedImage.downsize(path, path, "100x100", opts) - Implement a single method that supports both signatures (see original suggestion)
🧰 Tools
🪛 RuboCop (1.84.0)
[warning] 149-149: Method OptimizedImage.downsize is defined at both app/models/optimized_image.rb:145 and app/models/optimized_image.rb:149.
(Lint/DuplicateMethods)
🤖 Prompt for AI Agents
In `@app/models/optimized_image.rb` around lines 141 - 156, The duplicate downsize
definitions must be consolidated so callers like resize_emoji (which passes
three positional args for width/height) and uploads_controller (which passes a
dimensions string) both work: remove the duplicated method and implement a
single OptimizedImage.downsize that accepts both signatures (e.g., parameters
from, to, then either (max_width, max_height, opts={}) or (dimensions_string,
opts={}), detect whether the third arg is a String or a Numeric, normalize to a
dimensions string like "WxH", merge opts, and then call optimize(operation,
from, to, dimensions, opts); update references to method names used in optimize
(e.g., optimize and any *_instructions methods remain unchanged).
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit